nik4: init at 1.8 #366194

Luflosi · 2024-12-18T15:05:49Z

Add nik4, a program that can render a map from OpenStreetMap data as an image.
openstreetmap-carto is an OpenStreetMap style. It defines how the map looks.
Also disable the broken python3Packages.python-mapnik test, since I need this dependency. It seems to work fine for this use-case.
I also wrote a NixOS test to show how everything fits together. If you run it (and wait many minutes), you get a nice map in the output.

I may or may not have gone a bit overboard with the openstreetmap-carto package, where I made it not depend on the internet, so it can run in the NixOS VM test. I also regenerate all automatically generated files that I could find and check that we get the same files. I also separated different parts into their own outputs. This way the closure size should be reduced if you don't need certain parts. For example the get-external-data.py script only needs to be run once to import the map into the database and after that, its huge closure (over two gigabytes) is no longer needed (unless you want to update the map).
I chose to use the latest git revision of openstreetmap-carto because it supports the flex layout, unlike the latest release.

@NixOS/geospatial please review.
I think this is a super cool demo.

Things done

Add a 👍 reaction to pull requests you find important.

Luflosi · 2024-12-18T15:09:58Z

Also see https://nixcademy.com/posts/nixos-openstreetmap/ and https://github.com/tfc/nixos-openstreetmap, which does something similar.

Luflosi · 2024-12-18T15:14:01Z

I plan to create similar NixOS tests for calculating a route between two coordinates and also for full text search in the future.

imincik · 2024-12-19T13:00:50Z

Great work @Luflosi ! I'll review ASAP.

imincik

@Luflosi , thank you very much for interesting work and sorry for waiting so long for review.

I suggested some changes and might have a few comments later, but I'm pretty sure we get this merged soon !

BTW, you are very welcome to join Nix Geospatial Team.

pkgs/by-name/ni/nik4/package.nix

pkgs/by-name/op/openstreetmap-carto/package.nix

imincik · 2025-01-02T10:35:20Z

pkgs/by-name/op/openstreetmap-carto/package.nix

@@ -0,0 +1,210 @@
+{
+  carto,


Personally, I like when the list of function parameters is logically separated - lib and functions first and then packages (see gdal as an example).

imincik · 2025-01-02T11:01:09Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+
+    # The original get-fonts.sh is currently broken, see https://github.com/gravitystorm/openstreetmap-carto/issues/5013
+    # This original script also does not aim to reproducible. It does not do any commit pinning or checksumming.
+    get_fonts = writeShellApplication {


What is the purpose of get_fonts script ? Also, I would try to avoid to run nix CLI (nix-store) here.

The original get_fonts.sh script downloads fonts needed for properly rendering the map into a new directory in the current working directory. You can see an example of it being used (or at least prepared for use) here: https://github.com/tfc/nixos-openstreetmap/blob/0fd30b016eb838395d85948b9ecf00ff59b4581d/openstreetmap.nix#L67-L74. In that example renderd later makes use of these files.

I don't like the original script:

It's currently broken, see get-fonts fails, due to fonts.google.com not returning a zip. gravitystorm/openstreetmap-carto#5013

It spews warnings while running

It downloads old versions of the fonts from (among other places) a now archived repository

It doesn't do any checksumming or signature verification
So I replaced it with my own version which just reuses fonts already packaged in Nixpkgs. I'm not using it in the NixOS test because I can pass the fonts directly to Nik4, which is more declarative than running an imperative step first.
The reason why I chose to use the Nix CLI instead of copying the files was that I disliked the files being duplicated on disk. Nix just creates a symlink. But since you seem to really dislike using the Nix CLI, I replaced the call to nix with cp. I had to use --dereference since otherwise cp just copies the symlinks to the files in the Nix store themselves, which are not known to the garbage collector and will thus be deleted eventually.

imincik · 2025-01-02T11:02:49Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+    "out"
+    "sql"
+    "lua"
+    "get_external_data"


I suggest to rename this output.

Suggested change

"get_external_data"

"external_data"

The script is called get-external-data.py, so I named the output get_external_data. The output does not directly contain the external data but only a script that knows how to "get" it. Do you still think that I should rename it?

I am still in favor calling it external_data.

imincik · 2025-01-02T11:19:30Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+        inherit hash;
+      };
+    };
+  simplified_water_polygons = mkArchive {


What about moving all data downloading code to nixos/tests/nik4.nix test and generate external-data.yml file required for test script there ? It looks to me as a more appropriate place.

My goal was to make this script as reproducible as possible, no matter if it's called in the NixOS test or on a users system. I'm in favor of keeping it this way.

What is the purpose of this data ? Isn't it useful mostly for testing ? If that's true, I still lean more towards moving this code to the test.
Another issue of having this code in package is how to update this data over the time (and how often). We might end up with reproducible but very outdated data.

What do you think @autra ?

pkgs/development/python-modules/python-mapnik/default.nix

nixos/tests/nik4.nix

pkgs/by-name/op/openstreetmap-carto/package.nix

nixos/tests/nik4.nix

autra · 2025-01-16T15:33:04Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+
+    # The original get-fonts.sh is currently broken, see https://github.com/gravitystorm/openstreetmap-carto/issues/5013
+    # This original script also does not aim to be reproducible. It does not do any commit pinning or checksumming.
+    get-fonts = writeShellApplication {


We shouldn't need to copy them. If I understand correctly, create a store path with all the needed font with symlinkJoin like you did, then patching font.mss here to make it use it would be better (at least more aligned with what I've seen in nixpkgs).

In fact, the very principle of get-fonts is not really what we do with nix/nixos (because, as you said elsewhere, it is neither reproducible nor predictable).

Thanks for that insight. I didn't know it was possible to patch the OSM style to depend directly on the fonts. I thought you needed to pass the fonts to the rendering software separately (such as Nik4 or renderd).

I removed the script.

autra · 2025-01-16T15:34:18Z

I made 2 remarks, but things are already looking good! Thanks for starting this work :-)

imincik · 2025-01-20T10:08:35Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+  lib,
+  stdenvNoCC,
+  fetchFromGitHub,
+  fetchurl,
+
+  carto,
+  gdal,
+  hanazono,
+  nixosTests,
+  noto-fonts,
+  python3,
+  runCommand,
+  symlinkJoin,


Suggested change

lib,

stdenvNoCC,

fetchFromGitHub,

fetchurl,

carto,

gdal,

hanazono,

nixosTests,

noto-fonts,

python3,

runCommand,

symlinkJoin,

lib,

stdenvNoCC,

fetchFromGitHub,

fetchurl,

nixosTests,

runCommand,

symlinkJoin,

carto,

gdal,

hanazono,

noto-fonts,

python3,

imincik · 2025-01-20T10:09:48Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+    "out"
+    "sql"
+    "lua"
+    "get_external_data"


I am still in favor calling it external_data.

imincik · 2025-01-20T10:23:47Z

pkgs/by-name/op/openstreetmap-carto/package.nix

+        inherit hash;
+      };
+    };
+  simplified_water_polygons = mkArchive {


What is the purpose of this data ? Isn't it useful mostly for testing ? If that's true, I still lean more towards moving this code to the test.
Another issue of having this code in package is how to update this data over the time (and how often). We might end up with reproducible but very outdated data.

What do you think @autra ?

autra · 2025-01-21T11:06:45Z

What do you think @autra ?

I think we should usually not package data, especially those data because they change quite often I'm sure. There are exceptions (proj reprojection grids I guess, even though it's probably debatable).

Considering that upstream's logic is to let the user downloading data via get_external_data.py and that you can specify the config, it seems the external_data.yml is just an example file. Because of that, I'm even more against packaging those data in the store: users may and will want other data.

In short everything that is not in upstream repository should not be packaged imo.

However, we can package the script and the default config file (and even patch the script so that it defaults to this config file in the store if necessary).

For the derivation output, I'd package all relevant scripts in openstreetmap-carto/scripts in the ${out}/bin/ output (correctly wrapped so that they work out of the box in nixos). I don't see the point of having separate outputs for get_external_data. I don't think we need to wrap all these scripts now though, just package those you need today @Luflosi.

For other files (styles? fonts if necessary?) I don't mind another output, but I wouldn't mind a folder in the main output, but I admit that the advantages of having different outputs or only one are not totally clear to me

github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels Dec 18, 2024

github-actions bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Dec 18, 2024

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes labels Dec 19, 2024

imincik requested changes Jan 2, 2025

View reviewed changes

imincik reviewed Jan 2, 2025

View reviewed changes

pkgs/by-name/op/openstreetmap-carto/package.nix Outdated Show resolved Hide resolved

Luflosi force-pushed the add/nik4 branch from 9558f40 to aa633c4 Compare January 6, 2025 13:23

autra reviewed Jan 16, 2025

View reviewed changes

nixos/tests/nik4.nix Show resolved Hide resolved

autra reviewed Jan 16, 2025

View reviewed changes

Luflosi force-pushed the add/nik4 branch from aa633c4 to 879c55e Compare January 19, 2025 14:07

Luflosi added 4 commits January 19, 2025 15:32

python3Packages.python-mapnik: disable broken test

b8b31c5

openstreetmap-carto: init at 5.9.0-unstable-2024-11-27

b70af9e

nik4: init at 1.8

f3ee67e

nixos/tests/nik4: init

cc192b6

Luflosi force-pushed the add/nik4 branch from 879c55e to cc192b6 Compare January 19, 2025 14:35

Luflosi requested review from imincik and autra January 19, 2025 14:37

imincik requested changes Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nik4: init at 1.8 #366194

nik4: init at 1.8 #366194

Luflosi commented Dec 18, 2024

Luflosi commented Dec 18, 2024

Luflosi commented Dec 18, 2024

imincik commented Dec 19, 2024

imincik left a comment

imincik Jan 2, 2025

Luflosi Jan 6, 2025

imincik Jan 2, 2025

Luflosi Jan 6, 2025

imincik Jan 2, 2025

Luflosi Jan 6, 2025

imincik Jan 20, 2025

imincik Jan 2, 2025

Luflosi Jan 6, 2025

imincik Jan 20, 2025

autra Jan 16, 2025

Luflosi Jan 19, 2025

autra commented Jan 16, 2025

imincik Jan 20, 2025

imincik Jan 20, 2025

imincik Jan 20, 2025

autra commented Jan 21, 2025

nik4: init at 1.8 #366194

Are you sure you want to change the base?

nik4: init at 1.8 #366194

Conversation

Luflosi commented Dec 18, 2024

Things done

Luflosi commented Dec 18, 2024

Luflosi commented Dec 18, 2024

imincik commented Dec 19, 2024

imincik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autra commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autra commented Jan 21, 2025